Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands ECC-related integration testing in the C++ test suite and adjusts the Rust ECC pre-hashed ECDSA signing implementation to accept oversized output buffers (matching the C API’s “buffer is a capacity” contract).
Changes:
- Add extensive new C++ ECC sign/verify test coverage (binary payloads, buffer sizing sweeps, algorithm/curve matrices, and argument-validation scenarios).
- Expand C++ ECC key attestation (key report) tests with additional boundary and contract checks.
- Update Rust
HsmEccSignAlgo::sign()to treat signature buffers as “at least required size” (reject only when too small).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| api/tests/cpp/algo/ecc/sign_verify_tests.cpp | Large expansion of ECC sign/verify single-shot + streaming coverage and API contract tests. |
| api/tests/cpp/algo/ecc/keyreport_tests.cpp | Adds key report buffer/argument contract tests and additional handle-class coverage. |
| api/tests/cpp/algo/ecc/keygen_tests.cpp | Reorganizes/expands ECC keygen lifecycle tests and property-validation coverage. |
| api/lib/src/algo/ecc/sign.rs | Fixes ECC sign output-buffer sizing check to allow oversized buffers. |
| #include "handle/part_handle.hpp" | ||
| #include "handle/part_list_handle.hpp" | ||
| #include "handle/session_handle.hpp" | ||
| #include "../aes/helpers.hpp" |
api/native/Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| azihsm_api.workspace = true | ||
| azihsm_crypto.workspace = true |
c4adfd5 to
06c79e3
Compare
071d2c5 to
18408cd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
api/native/src/algo/rsa/key.rs:1
- validate_rsa_aes_wrap_params(params) is now called both inside the TryFrom<&AzihsmAlgo> implementation and again in rsa_unwrap_key / rsa_unwrap_key_pair. This duplicates work and increases the chance of the validations diverging over time. Consider removing the second validation in the unwrap functions and relying on the TryFrom path as the single validation gate.
// Copyright (c) Microsoft Corporation.
api/native/src/algo/rsa/key.rs:1
- validate_rsa_aes_wrap_params(params) is now called both inside the TryFrom<&AzihsmAlgo> implementation and again in rsa_unwrap_key / rsa_unwrap_key_pair. This duplicates work and increases the chance of the validations diverging over time. Consider removing the second validation in the unwrap functions and relying on the TryFrom path as the single validation gate.
// Copyright (c) Microsoft Corporation.
de11200 to
00de38f
Compare
a416baa to
a1a5b87
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
api/native/doc/chapter_09_data_structs.md:558
- The OAEP docs here refer to the field name
mgf_id, but the rest of the SDK/test code usesmgf1_hash_algo_idforazihsm_algo_rsa_pkcs_oaep_params(and the table below labels it asmgf1). Please align this section’s field naming with the actual C struct field name to avoid confusion for API consumers.
**Current SDK limitation:** `hash_algo_id` and `mgf_id` must use the same hash function.
Passing mixed OAEP hash/MGF1 combinations returns `AZIHSM_STATUS_INVALID_ARGUMENT`.
```cpp
struct azihsm_algo_rsa_pkcs_oaep_params {
azihsm_algo_id hash_algo_id;
azihsm_mgf1_id mgf_id;
const azihsm_buffer *label;
| oaep_params.mgf1_hash_algo_id, | ||
| oaep_params.label, | ||
| )?; | ||
|
|
api/native/src/algo/rsa/key.rs
Outdated
| @@ -36,6 +36,8 @@ impl<'a> TryFrom<&'a AzihsmAlgo> for &'a AzihsmAlgoRsaAesKeyWrapParams { | |||
| // Validate OAEP parameters pointer | |||
| validate_ptr(params.oaep_params)?; | |||
There was a problem hiding this comment.
this is redundant call as deref_ptr in validate_rsa_aes_wrap_params does the check
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
| if (std::string(ex.what()).find("Error: -8") != std::string::npos) | ||
| { | ||
| skipped_due_to_session_capacity = true; | ||
| return; | ||
| } |
| { | ||
| if (std::string(ex.what()).find("Error: -8") != std::string::npos) | ||
| { | ||
| skipped_due_to_session_capacity = true; | ||
| return; |
| catch (const std::exception &ex) | ||
| { | ||
| if (std::string(ex.what()).find("Error: -8") != std::string::npos) | ||
| { | ||
| skipped_due_to_session_capacity = true; |
aa020b3 to
1ffe694
Compare
No description provided.